Improve "SENTINEL FAILOVER" by using the "FAILOVER" command #1292
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1292 +/- ##
============================================
- Coverage 70.76% 70.55% -0.21%
============================================
Files 116 119 +3
Lines 63300 64787 +1487
============================================
+ Hits 44793 45713 +920
- Misses 18507 19074 +567
|
gmbnomis
force-pushed
the
sentinel_coord_failover
branch
2 times, most recently
from
506b361 to
74ddfd4
Compare
gmbnomis
force-pushed
the
sentinel_coord_failover
branch
from
74ddfd4 to
55473d8
Compare
|
@enjoy-binbin I don't want to be pushy, but this PR is over a month old now. Could you have a look at the PR and/or #1291 when you find some time? |
|
sorry, i dont have enough time to review this, i guess i will be free in the next month. @hwware wen, do you think you have the time to review this in a short time? since you know the sentinel better than me. |
|
Sure, let me review it. @enjoy-binbin |
@gmbnomis Sorry, let me review it ASAP. |
|
Could you please provide a CI result which include sentinel test suites, because there is no sentinel test in the commit CI? Thanks |
| assert_equal {OK} [S $id SENTINEL CONFIG SET resolve-hostnames no announce-port 0] | ||
| } | ||
| assert_match {*no*0*} [S 1 SENTINEL CONFIG GET resolve-hostnames announce-port] | ||
| assert_match {announce-port 0} [S 1 SENTINEL CONFIG GET announce-port] |
There was a problem hiding this comment.
Why we need this test? and why we need Line 65 again to get announce-port second time?
There was a problem hiding this comment.
It turns out that the sentinel test framework does not run tests completely independent. Configuration changes like the ones tested in this test are persisted and used in the following tests.
I considered three options:
- Change the test framework: While this might be a good idea, I did not want to do this in this PR
- Undo the config changes at the end of the "config-set-config-get" test.
- Keep "config-set-config-get" at the end and insert the new tests "manual-coordinated" and "stuck-failover" in the middle, e.g. after "05-manual"
I took option 2 and that's why this code is here. Code lines 64 and 65 just reflect what's done in lines 7 and 8 of the file. You are right that line 65 is redundant in this case and can be deleted.
Alternatively, I could try option 3 and rename the test files.
There was a problem hiding this comment.
I deleted line 65 for the time being.
Here is a manual run of the Daily job test-ubuntu-jemalloc with sentinel tests enabled. Is this what you are looking for? If not, please let me know what I should run. |
Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
Up to now the sentinel test initialization verified that all sentinels detect each other. However, detection does not imply connection, which led to intermittent failures in the coordinated failover tests (no leader elected since disconnected sentinels do not take part in a vote). Fix this by waiting until no sentinel reports being "disconnected". Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
Valkey supports the "FAILOVER" command to switch primary and replica roles in a coordinated fashion. Add a "COORDINATED" option to "SENTINEL FAILOVER". When given, use "FAILOVER" in the Sentinel forced failover procedure. Since we force the failover, i.e. we don't elect a leader, the failover must succeed quickly before other Sentinels will regard the primary as down. Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
The FAILOVER command may ["encounter some scenarios it can not automatically remediate from and may get stuck."](https://valkey.io/commands/failover/#failover-abort). The supervision of the Sentinels is used to handle these situations: 1. "REPLICAOF" is not accepted during a failover. Thus, send a "FAILOVER ABORT" before sending a "REPLICAOF" to a node in `sentinelSendReplicaOf()`. (If there is no ongoing failover, the resulting error will just be ignored) 2. Sentinel monitors nodes for deviations from the expected state (wrong role or replication). Add a check for replicas that are in a failover state for too long and reconfigure them. In addition, monitoring the failover state is used to find out whether the FAILOVER command is supported by the Valkey instance. If not, don't use "FAILOVER" or "FAILOVER ABORT". Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
Doing a proper leader election allows to increase the time available for the actual FAILOVER command, as other Sentinels will not attempt to initiate another failover during the failover timeout. Interestingly, Sentinel does not take the reported primary up/down status into account when counting replies to leader election. Thus, Sentinel will simply proceed with the failover once we reach the quorum even if we don't are in O_DOWN state. Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
gmbnomis
force-pushed
the
sentinel_coord_failover
branch
from
55473d8 to
d5fadec
Compare
|
@hwware Thank you for your review! Given that this PR has been inactive for an additional month, could you please provide guidance on how we can move forward with getting it merged? |
This PR implements #1291 and consists of the following commits:
Two commits to fix problems with the sentinel tests:
Commit: Wait for all Sentinels to be connected before starting tests
Up to now the sentinel test initialization verified that all sentinels detect each other.
However, detection does not imply connection, which led to intermittent failures in the
coordinated failover tests (no leader elected since disconnected sentinels do not take part in a
vote).
Fix this by waiting until no sentinel reports being "disconnected".
Commit: sentinel-tests: Clean up config after config set tests
Three commits with the actual implementation:
Commit: Add option for coordinated failover to Sentinel (coordinated failover without leader election)
Commit: Allow Sentinel to recover from a stuck FAILOVER
Commit: SENTINEL FAILOVER COORDINATED actually does a leader election (adds leader election before failover starts)